-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[ASYNCIFY] Run continuations under callUserCallback #26095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
b559688 to
6122ec5
Compare
6122ec5 to
ee1f567
Compare
|
This change is going to be useful / essential for the followup to #26019 where i hope to make all (most) asyncify usage in the JS library automatic/transparent. |
ee1f567 to
20d5e0b
Compare
| ] | ||
| self.cflags += args | ||
| self.do_core_test('embind_lib_with_asyncify.cpp') | ||
| self.do_core_test('embind_lib_with_asyncify.cpp', assert_returncode=NON_ZERO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that because the asyncify continuation are now run in callUserCallback the handleException: got unexpected exception, calling quit_` which sets the node exit code to non-zero.
This test explicit has delayed_throw in it which throws and exception in an asyncify continuation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that delayed throw is handled early in the test,
let delayedThrowResult = Module["delayed_throw"]();
assert(delayedThrowResult instanceof Promise);
let err = await delayedThrowResult.then(() => '', err => err.message);
assert(err === 'my message', `"${err}" doesn't contain the expected error`);The rest of the test runs after those lines, and it exits without error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .then(() => '', err => err.message); part handles the error, but the exit code is remembered because handleExcetpion calls quit_ which under node sets the overall process exit code:
Line 253 in 48a147b
| process.exitCode = status; |
So its kind of like the node implementation is remember the exit code even though the exception was handled further up. I don't know if this is ideal, but I imagine changing it would be very hard at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Yeah, probably not worth changing.
How about adding a comment in the python file, explaining why we expect the test to fail? Otherwise it is quite surprising I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
20d5e0b to
257a6ad
Compare
257a6ad to
d354cfd
Compare
Without this, its up to each callsite to inject
callUserCallback(e.g. viasafeSetTimeoutvssetTimeout).Forgetting to do so can resulting in strange behaviour such as
exit()not work as expected when called from the continuation.